Make sure published messages are acknowledged for play mode #951

Barry-Xu-2018 · 2022-01-27T03:31:25Z

Address #571

After test with the latest code for rolling (use fastdds), #571 (follow steps to reproduce in #571) cannot reproduce.
Note that synchronous is set as default for fastdds recently(ros2/rmw_fastrtps@e7c749a). But even if I use async mode, #571 also cannot reproduce.

Signed-off-by: Barry Xu <barry.xu@sony.com>

Barry-Xu-2018 · 2022-01-27T04:38:53Z

The failure on ament_uncrustify is related to file mock_storage.hpp, which isn't changed in this PR.

--- rosbag2_compression/test/rosbag2_compression/mock_storage.hpp
+++ rosbag2_compression/test/rosbag2_compression/mock_storage.hpp.uncrustify
@@ -43 +43 @@
-    void(const std::vector<std::shared_ptr<const rosbag2_storage::SerializedBagMessage>> &));
+    void(const std::vector<std::shared_ptr<const rosbag2_storage::SerializedBagMessage>>&));

Code style divergence in file 'rosbag2_cpp/test/rosbag2_cpp/mock_storage.hpp':

--- rosbag2_cpp/test/rosbag2_cpp/mock_storage.hpp
+++ rosbag2_cpp/test/rosbag2_cpp/mock_storage.hpp.uncrustify
@@ -44 +44 @@
-    void(const std::vector<std::shared_ptr<const rosbag2_storage::SerializedBagMessage>> &));
+    void(const std::vector<std::shared_ptr<const rosbag2_storage::SerializedBagMessage>>&));

2 files with code style divergence
Error: The process '/usr/bin/bash' failed with exit code 1

Barry-Xu-2018 · 2022-01-27T05:02:23Z

@fujitatomoya @emersonknapp
Please help to review.

fujitatomoya · 2022-01-27T06:02:31Z

@Barry-Xu-2018 will do in a couple of days.

jhdcs

Looks good to me. I only saw a couple comments that might use a bit of tweaking, though I don't think it's enough to hold up the review over.

Since you requested @fujitatomoya and @emersonknapp as reviewers, specifically, I'll wait on their judgement rather than attempt merging for now.

jhdcs · 2022-01-27T12:53:20Z

rosbag2_transport/src/rosbag2_transport/player.cpp

@@ -229,6 +229,30 @@ void Player::play()
  std::lock_guard<std::mutex> lk(ready_to_play_from_queue_mutex_);
  is_ready_to_play_from_queue_ = false;
  ready_to_play_from_queue_cv_.notify_all();
+
+  // Wait for all published messages are acknowledged


Suggested change

// Wait for all published messages are acknowledged

// Wait for all published messages to be acknowledged

jhdcs · 2022-01-27T12:54:55Z

rosbag2_transport/include/rosbag2_transport/play_options.hpp

+  // Timeout for waiting all published messages acknowledged
+  // negative means published messages need not to be acknowledged


Suggested change

// Timeout for waiting all published messages acknowledged

// negative means published messages need not to be acknowledged

// Timeout for waiting for all published messages to be acknowledged.

// Negative values means that published messages do not need to be acknowledged.

fujitatomoya · 2022-01-27T21:17:35Z

@jhdcs since i do not have any authorization on this repository, you can just go ahead instead of my comments if you are good to go. (i will do the review as community reviewer 😃 )

fujitatomoya · 2022-01-27T21:21:53Z

ros2bag/ros2bag/verb/play.py

+def not_negative_int(arg: str) -> int:
+    value = int(arg)
+    if value < 0:
+        raise ArgumentTypeError(f'Value {value} is less than zero.')
+    return value
+
+


why dont we have this along with check_positive_float as common function?

According to my understanding, the input parameter of check function is fixed (only arg: str, and this parameter is passed by argparse module).
We cannot use this parameter to specify what to do, such as check positive or not_negative.
So I have no idea how to use one common function to implement check_positive_float and not_negative_int.

i was not clear on this, i was just saying to move this function where check_positive_float is.

i was just saying to move this function where check_positive_float is.

Get it.

fujitatomoya · 2022-01-27T21:40:13Z

rosbag2_transport/src/rosbag2_transport/player.cpp

+        RCLCPP_WARN(
+          get_logger(),
+          "--wait-for-all-acked is invaild for topic '%s' since reliability of QOS is BestEffort.",
+          topic.name.c_str());


if 100 of 200 topics come true with this if statement, there will be 100 warning lines here. which btw, i think it is likely to happen. do we need to print this warning for each topic? how about printing warning once that --wait-for-all-acked is invalid for topics with reliability of QOS is BestEffort.?

if 100 of 200 topics come true with this if statement, there will be 100 warning lines here.

Yes. It is poor user experience.
But I think topic information is useful for user.
How about change as below: (only print one warning for all invalid topics)

-wait-for-all-acked is invalid for below topics with reliability of QOS is BestEffort xxx, xxx, xxx, xxx, xxx

fujitatomoya · 2022-01-27T21:42:24Z

ros2bag/ros2bag/verb/play.py

@@ -93,6 +101,17 @@ def add_arguments(self, parser, cli_name):  # noqa: D102
        parser.add_argument(
            '--start-offset', type=check_positive_float, default=0.0,
            help='Start the playback player this many seconds into the bag file.')
+        parser.add_argument(
+            '--wait-for-all-acked', type=not_negative_int, default=0,


default behavior would be okay with wait forever to avoid possible problem to miss the last message? we would like to have consensus on this. CC: @emersonknapp @jhdcs
IMO, we would want to have specific timeout like 1 sec instead of wait forever, because if anything happens, wait forever will be not good for user experience.

Yeah.
I think this option should be disabled by default, since most cases don't need this feature.
What do you think ?

fujitatomoya · 2022-01-27T23:52:38Z

rosbag2_transport/src/rosbag2_transport/player.cpp

+        if (!pub.second->wait_for_all_acked(timeout)) {
+          RCLCPP_ERROR(
+            get_logger(),
+            "Failed to wait all published messages acknowledged for topic %s",


this happens only when timeout, so how about the following? I think timeout is different from failure or exception.

Suggested change

"Failed to wait all published messages acknowledged for topic %s",

"Timeout to wait all published messages acknowledged for topic %s",

Signed-off-by: Barry Xu <barry.xu@sony.com>

jhdcs

Did another review pass, since several changes were made. Again, just had some comment/documentation suggestions.

jhdcs · 2022-01-28T15:49:32Z

ros2bag/ros2bag/verb/play.py

+            '--wait-for-all-acked', type=check_not_negative_int, default=-1,
+            help='Wait until all published messages are acknowledged by all subscribers or until '
+                 'the timeout elapses in millisecond before play is terminated. '
+                 'Especially for the case of sending message with big size in a short time. '


I feel like this flows a little better, though I tend to be a bit wordy.

Suggested change

'Especially for the case of sending message with big size in a short time. '

'Especially useful when sending many messages with large sizes in a short timeframe.'

jhdcs · 2022-01-28T15:51:56Z

rosbag2_transport/src/rosbag2_transport/player.cpp

+        if (!pub.second->wait_for_all_acked(timeout)) {
+          RCLCPP_ERROR(
+            get_logger(),
+            "Timeout to wait all published messages acknowledged for topic %s",


I feel like this reads a little better.

Suggested change

"Timeout to wait all published messages acknowledged for topic %s",

"Timed out while waiting for all published messages to be acknowledged for topic %s",

jhdcs · 2022-01-28T15:53:05Z

rosbag2_transport/src/rosbag2_transport/player.cpp

+      } catch (std::exception & e) {
+        RCLCPP_ERROR(
+          get_logger(),
+          "Failed to wait all published messages acknowledged for topic %s : %s",


Another small wordage suggestion.

Suggested change

"Failed to wait all published messages acknowledged for topic %s : %s",

"Exception occurred while waiting for all published messages to be acknowledged for topic %s : %s",

jhdcs · 2022-01-28T15:53:33Z

rosbag2_transport/src/rosbag2_transport/player.cpp

+
+    RCLCPP_WARN(
+      get_logger(),
+      "--wait-for-all-acked is invaild for below topics since reliability of QOS is BestEffort.\n"


Very minor change here.

Suggested change

"--wait-for-all-acked is invaild for below topics since reliability of QOS is BestEffort.\n"

"--wait-for-all-acked is invaild for the below topics since reliability of QOS is BestEffort.\n"

Thanks for your comments.
Please help to check again @jhdcs

Signed-off-by: Barry Xu <barry.xu@sony.com>

jhdcs

Looks good to me!

clalancette · 2022-02-01T13:24:26Z

Looks good to me!

Did CI from https://ci.ros2.org get run on this PR?

jhdcs · 2022-02-01T13:25:08Z

Oh crud, I thought it had!

Crud, let me see how to undo that...

…951)" This reverts commit 9447f4e.

jhdcs · 2022-02-01T13:26:45Z

Not sure I did that right...

Sorry, first time merging a PR...

clalancette · 2022-02-01T14:26:58Z

Here's CI after the merge:

Linux
Linux-aarch64
Windows

fujitatomoya · 2022-02-01T16:44:49Z

@clalancette thanks for checking on this one. after all, everything looks okay!

Make sure published messages are acknowledged for play mode

43fe65e

Signed-off-by: Barry Xu <barry.xu@sony.com>

Barry-Xu-2018 requested a review from a team as a code owner January 27, 2022 03:31

Barry-Xu-2018 requested review from emersonknapp and jhdcs and removed request for a team January 27, 2022 03:31

jhdcs approved these changes Jan 27, 2022

View reviewed changes

fujitatomoya reviewed Jan 27, 2022

View reviewed changes

Barry-Xu-2018 added 2 commits January 28, 2022 14:07

Address review comments

96ae900

Signed-off-by: Barry Xu <barry.xu@sony.com>

Move check_not_negative() to __init__.py

e45d1ed

Signed-off-by: Barry Xu <barry.xu@sony.com>

jhdcs approved these changes Jan 28, 2022

View reviewed changes

Address review comments

7cc4fcb

Signed-off-by: Barry Xu <barry.xu@sony.com>

jhdcs approved these changes Feb 1, 2022

View reviewed changes

jhdcs merged commit 9447f4e into ros2:master Feb 1, 2022

jhdcs added a commit that referenced this pull request Feb 1, 2022

Revert "Make sure published messages are acknowledged for play mode (#…

223f78e

…951)" This reverts commit 9447f4e.

jhdcs mentioned this pull request Feb 1, 2022

Revert "Make sure published messages are acknowledged for play mode" #954

Closed

emersonknapp mentioned this pull request Feb 3, 2022

While playing, the last recorded message cannot be sent if the size of the message is big. #571

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure published messages are acknowledged for play mode #951

Make sure published messages are acknowledged for play mode #951

Barry-Xu-2018 commented Jan 27, 2022 •

edited

Loading

Barry-Xu-2018 commented Jan 27, 2022

Barry-Xu-2018 commented Jan 27, 2022

fujitatomoya commented Jan 27, 2022

jhdcs left a comment

jhdcs Jan 27, 2022

jhdcs Jan 27, 2022

fujitatomoya commented Jan 27, 2022

fujitatomoya Jan 27, 2022

Barry-Xu-2018 Jan 28, 2022

fujitatomoya Jan 28, 2022

Barry-Xu-2018 Jan 28, 2022

fujitatomoya Jan 27, 2022

Barry-Xu-2018 Jan 28, 2022

fujitatomoya Jan 27, 2022

Barry-Xu-2018 Jan 28, 2022

fujitatomoya Jan 27, 2022

jhdcs left a comment

jhdcs Jan 28, 2022

jhdcs Jan 28, 2022

jhdcs Jan 28, 2022

jhdcs Jan 28, 2022

Barry-Xu-2018 Feb 1, 2022

jhdcs left a comment

clalancette commented Feb 1, 2022

jhdcs commented Feb 1, 2022

jhdcs commented Feb 1, 2022

clalancette commented Feb 1, 2022

fujitatomoya commented Feb 1, 2022

	// Wait for all published messages are acknowledged
	// Wait for all published messages to be acknowledged

		// Timeout for waiting all published messages acknowledged
		// negative means published messages need not to be acknowledged

	"Failed to wait all published messages acknowledged for topic %s",
	"Timeout to wait all published messages acknowledged for topic %s",

	'Especially for the case of sending message with big size in a short time. '
	'Especially useful when sending many messages with large sizes in a short timeframe.'

	"Timeout to wait all published messages acknowledged for topic %s",
	"Timed out while waiting for all published messages to be acknowledged for topic %s",

	"Failed to wait all published messages acknowledged for topic %s : %s",
	"Exception occurred while waiting for all published messages to be acknowledged for topic %s : %s",

	"--wait-for-all-acked is invaild for below topics since reliability of QOS is BestEffort.\n"
	"--wait-for-all-acked is invaild for the below topics since reliability of QOS is BestEffort.\n"

Make sure published messages are acknowledged for play mode #951

Make sure published messages are acknowledged for play mode #951

Conversation

Barry-Xu-2018 commented Jan 27, 2022 • edited Loading

Barry-Xu-2018 commented Jan 27, 2022

Barry-Xu-2018 commented Jan 27, 2022

fujitatomoya commented Jan 27, 2022

jhdcs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fujitatomoya commented Jan 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhdcs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhdcs left a comment

Choose a reason for hiding this comment

clalancette commented Feb 1, 2022

jhdcs commented Feb 1, 2022

jhdcs commented Feb 1, 2022

clalancette commented Feb 1, 2022

fujitatomoya commented Feb 1, 2022

Barry-Xu-2018 commented Jan 27, 2022 •

edited

Loading